Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIRRTL] AdvancedLayerSink: don't sink instances of mods with port annos #7982

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

rwy7
Copy link
Contributor

@rwy7 rwy7 commented Dec 13, 2024

If a module-like has port annotations, mark it as an effectful module, which will prevent the pass from moving or deleting its instances later on. We can't sink instance when we don't understand what port annotations mean.

@rwy7 rwy7 requested review from youngar and dtzSiFive December 13, 2024 02:41
@rwy7 rwy7 force-pushed the fix-advanced-layer-sink branch from cbfb5a2 to 1a4a875 Compare December 13, 2024 02:49
Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

test/Dialect/FIRRTL/advanced-layer-sink.mlir Show resolved Hide resolved
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lib/Dialect/FIRRTL/Transforms/AdvancedLayerSink.cpp Outdated Show resolved Hide resolved
… annos

If a module-like has port annotations, mark it as containing an effect, which
will prevent the pass from moving or deleting its instances later on.
@rwy7 rwy7 force-pushed the fix-advanced-layer-sink branch from 65e1b7d to 9f3752b Compare December 13, 2024 13:42
@rwy7 rwy7 merged commit 6b4bea0 into llvm:main Dec 13, 2024
4 checks passed
@rwy7 rwy7 deleted the fix-advanced-layer-sink branch December 13, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants